^

Should we add a fillV3Relay overloaded function where the default repayment address is msg.sender? Could this avoid some breaking changes for existing fillers to give them time to migrate to new functions? Or will they already be broken by all the changes in the V3RelayData struct, for example the address -> bytes32 changes.

Yes, the changes in V3RelayData would also be breaking changes, so an update would still be necessary. I’m leaning towards only adding backward compatibility to functions that are called by other contracts, like depositV3.

I think the changes in V3Relay might not be as breaking necessarily since address can be implictly converted to bytes32. But I take your point.

@pxrl wdyt about this?

@pxrl how much lift do you think there will be for the executor to deal with this change where refunds can now fail?

note that they dont fail. the behaviour is more kind to the executor. in a very rare event where you set a blacklisted recipient in the transfer then the recipient gets a credit they can claim. the design here should require no additional changes to the executor. we should, however, listen to events that indicate if this has happened.

+1 for reverting here, as this is a common failure path where a spoke pool is missing funds due to some out of protocol issue like a l1->l2 token transfer via canonical bridge failing

Why does the hash need to be different?

Because, when adding backwards compatibility to speedUpV3Deposit with the address version, we still need to support an updatedMessage constructed with the old data type. Maybe this is unnecessary?

I was assuming that there might be cases were contracts are calling this speedUpV3Deposit with the old signature and thus passing updatedMessage built with the old data structure

However if speedUpV3Deposit is only expected to be called off-chain we probably don't need backwards compatibility and can drop UPDATE_V3_DEPOSIT_ADDRESS_OVERLOAD_DETAILS_HASH.

Can you pls clarify how is speedUpV3Deposit is used @nicholaspai ?

I was assuming that there might be cases were contracts are calling this speedUpV3Deposit with the old signature

Yeah this is possible for a short time after the contracts are upgraded, but long-term there will be no more speedUpV3Deposit calls with the old signature so we can actually mark this old signature as something to remove in the future

However if speedUpV3Deposit is only expected to be called off-chain

What do you mean by off-chain?

Can you pls clarify how is speedUpV3Deposit is used @nicholaspai ?

The way this is used is a depositor sees a speed up button on the FE which prompts them to sign a message which they then submit to speedUpV3Deposit, code here

So I see refundAddress is type address, and the relayerRefund mapping also uses an address=>address key. This is because these variables are only used on the chain that this spoke pool would be deployed to, which will always be an EVM?

I think we can migrate this to bytes32 as well to be consistent with the rest of updates. What do you think @chrismaree?

I agree for consistency it should be moved over. doing it now & updating the tests.

have updated this accordingly.

are there any storage/gas cost considerations to think about with this change?

I think it could be useful to emit the translated addresses at very little additional marginal cost

Why is setEnableRoute a specific case here? Is it the only function we'll be calling on the Solana spoke pool? For example, does this prevent us from pausing deposits/fills, emergency deleting root bundles, or forcing through admin root bundles? These haven't really happened. much in the past but the latter two have happened at least once

We need to keep the address version of setEnableRoute for backward compatibility with the hub pool, and that specific handler translates the message to the bytes32 version.

@Reinis-FRP, do you remember why we decided to handle the translation here, which led us to do the casting here instead of handling it in SVM?

The main reason is that setEnableRoute is invoked via setDepositRoute on HubPool that we are not upgrading and we need different types on Solana. For other admin methods they are crafted manually and bridged via generic relaySpokePoolAdminFunction method, so the operator can adjust the interface types for all other methods to Solana.

But @Reinis-FRP , couldn’t this conversion also be handled on the svm side? We initially chose to implement it here, but after thinking it over, I agree with @mrice32 that it might make more sense to handle this directly within the solana specific implementation. Also, @mrice32 pointed out a similar approach for Arbitrum in this comment.

This would be bit tricky as we would need to pass mapping account that stores address translation when receiving the message on Solana side. It does not really belong under HandleReceiveMessage which translates all other supported admin calls statelessly. And if we move the address translation account down to self-invoked SetEnableRoute then we would end up changing the interface of set_enable_route to use [u8; 20] as its origin_token parameter, which is not ideal as we also support calling it from Solana admin (or need to implement both flavors). And this also requires additional admin method to update the mapping account with correct address translation.

Not saying its impossible to do, but it seemed to have less code complexity if we move the translation to the adapter on EVM side, especially, since it already needs to translate trimmed Solana USDC address to its vault associated token account anyway.

So it sounds like the tradeoff here is (less code complexity, special case for setEnableDepositRoute on the EVM side, generic SVM side logic) versus (more code complexity, but totally generic EVM logic and bespoke SVM side logic), right? What about gas costs?

Why do we need to convert this to bytes32? Isn't this mapping from an origin chain token (which will always be a 20 byte address)?

Given that all the arguments have been moved to bytes32-- a decision that you can challenge as well--this made sense from a consistency perspective. We also decided to emit all addresses in their bytes32 form, so keeping the address representation here would require extra casting. Does this make sense @mrice32 ?

ye, there is an overall design here to make everything bytes32 internally this makes reasoning about data structures and props easier and more consistent as its always bytes32. we could in theory change this accordingly such that we treat all evm data as address type and type cast as soon as possible when entering methods with bytes32. very much opinonated, very much can be changed if you'll prefer the other option.

there is an overall design here to make everything bytes32 internally

I think this is correct directionally

Interface design question: if we know this token is local (and therefore an address), is there a point in making it bytes32?

Same question for depositor.

There may be an advantage in keeping them consistent, but there is a disadvantage in validation/error cases: it's possible to pass in bytes32 addresses that do not correspond to a valid local address.

The main reason was consistency with the rest of the data. We still have the overloaded versions of these functions that can help with the validation/casting of addresses.

We have tried to move everything to bytes32 in this contract. While in the short term there might be a bit of friction, I think in the medium term it’s better to have everything in either bytes32 or address, but not mixed, as mixing is more error-prone. However, we have made these changes somewhat radically and are looking for your feedback.

We do have bytes32 address encoding checks

I still think we may want to consider just forwarding the raw msg.data to the other function via a self delegatecall. I think this would be cheaper and less bytecode (although I could be wrong).

I'll look into this and report back.

I played with this for a few hours and think that the msg.data + self delegate call is overly complex and not worth it. I did something like this:

fallback() external payable {
        // Extract the function selector
        bytes4 selector;
        assembly {
            selector := calldataload(0)
        }

        // Check if selector matches `depositV3(address, address, address, address, uint256, uint256, uint256, address, uint32, uint32, uint32, bytes)`
        if (
            selector ==
            bytes4(
                keccak256(
                    "depositV3(address,address,address,address,uint256,uint256,uint256,address,uint32,uint32,uint32,bytes)"
                )
            )
        ) {
            // Extract each address parameter and cast to bytes32
            bytes32 depositor;
            bytes32 recipient;
            bytes32 inputToken;
            bytes32 outputToken;
            bytes32 exclusiveRelayer;

            // Using assembly for efficient calldata extraction and casting
            // Using assembly for efficient calldata extraction and conversion
            assembly {
                depositor := shl(96, calldataload(4)) // address -> bytes32
                recipient := shl(96, calldataload(36)) // address -> bytes32
                inputToken := shl(96, calldataload(68)) // address -> bytes32
                outputToken := shl(96, calldataload(100)) // address -> bytes32
                exclusiveRelayer := shl(96, calldataload(228)) // address -> bytes32
            }

            // Load other uint256 and uint32 values from calldata directly
            uint256 inputAmount;
            uint256 outputAmount;
            uint256 destinationChainId;
            uint32 quoteTimestamp;
            uint32 fillDeadline;
            uint32 exclusivityPeriod;
            bytes memory message;

            // Directly load the rest of the parameters from calldata
            assembly {
                inputAmount := calldataload(132)
                outputAmount := calldataload(164)
                destinationChainId := calldataload(196)
                quoteTimestamp := calldataload(260)
                fillDeadline := calldataload(292)
                exclusivityPeriod := calldataload(324)
            }

            // uint256 messageStart = 356; // Correct starting position for message
            // uint256 messageLength = msg.data.length > messageStart ? msg.data.length - messageStart : 0;

            // Copy `message` data directly
            // message = new bytes(messageLength);
            // if (messageLength > 0) {
            //     for (uint i = 0; i < messageLength; i++) {
            //         message[i] = msg.data[messageStart + i];
            //     }
            // }
            message = new bytes(0);

            // Forward the call to `depositV3(bytes32, bytes32, bytes32, bytes32, uint256, uint256, uint256, bytes32, uint32, uint32, uint32, bytes)
            (bool success, ) = address(this).delegatecall(
                abi.encodeWithSelector(
                    this.depositV3.selector,
                    depositor,
                    recipient,
                    inputToken,
                    outputToken,
                    inputAmount,
                    outputAmount,
                    destinationChainId,
                    exclusiveRelayer,
                    quoteTimestamp,
                    fillDeadline,
                    exclusivityPeriod,
                    message
                )
            );
            require(success, "Delegatecall to depositV3 failed");
        } else {
            // Handle unexpected calls
            revert("Function selector not supported");
        }
    }

(note the message decoding is broken so I hard coded it to bytes 0).

this works. but:

1. it's more expensive than the wrapped version (marginally, but still more).
2. it was super irritating to implement,
3. it makes our ABIs a massive pain to work with (we need a custom interface in the build process to append this to the ABI as we no longer have public functions exposing them

is this the kind of thing you were envisioning? I think if this is the pattern we need to go for then I'm a strong proponent of the much easier version that the PR has rn.

How much of this contract is specific to Solana vs could be used with any chain we connect to via CCTP?

I haven't gone deep on the specifics, but, for instance, can we build an adapter that could be used just as well with an EVM chain like BSC?

It is quite specific in a sense that it must be aware of Solana token model where recipient SpokePool address needs to be translated to its corresponding vault token address.

OOC, why not do this mapping on the Solana side?

In theory, could we not have a mapping (from truncated address to full address) exist on the Solana side that could be modified by an admin call. Then, the flow to add a new token would require no contract changes:

1. Send admin call to update the mapping with a new element.
2. Add the route to use the newly added element.

Side note: this is somewhat similar to something we have to do on arbitrum.

We still need to map truncated USDC address to Solana spoke pool vault here to support relayTokens method used when rebalancing from HubPool to Solana spoke via CCTP token bridge. So if we were to add new token we would anyways need to redeploy the adapter, hence it is easier to keep all related aliasing contained in this contract.